process: fix `process.features.typescript` when Amaro is unavailable #55323

aduh95 · 2024-10-08T16:49:34Z

nodejs-github-bot · 2024-10-08T16:49:38Z

Review requested:

@nodejs/startup

test/es-module/test-typescript.mjs

richardlau · 2024-10-08T17:14:57Z

$ ./node -p process.config.variables.node_use_amaro
false
$ ./node -p process.features.typescript
false
$

🎉

FWIW, with (and without) this PR I get this:

$ ./node --experimental-transform-types -p process.features.typescript
node:internal/bootstrap/realm:429
  if (!mod) throw new TypeError(`Missing internal module '${id}'`);
            ^

TypeError: Missing internal module 'internal/deps/amaro/dist/index'
    at requireBuiltin (node:internal/bootstrap/realm:429:19)
    at loadTypeScriptParser (node:internal/modules/helpers:346:19)
    at stripTypeScriptTypes (node:internal/modules/helpers:368:17)
    at node:internal/main/eval_string:27:3

Node.js v23.0.0-pre
$

If I try to use --experimental-transform-types (or --experimental-strip-types), which is coming from

node/lib/internal/modules/helpers.js

Line 346 in accb239

const amaro = require('internal/deps/amaro/dist/index');

.

Just running with the experimental options but dropping into the REPL doesn't throw:

$ ./node --experimental-transform-types
Welcome to Node.js v23.0.0-pre.
Type ".help" for more information.
> (node:95351) ExperimentalWarning: Type Stripping is an experimental feature and might change at any time
(Use `node --trace-warnings ...` to show where the warning was created)

>

aduh95 · 2024-10-08T17:17:32Z

FWIW, with (and without) this PR I get this:

I think that's because it assumes the string you pass is TS. Can you try the following instead:

echo 'console.log(process.features.typescript)' > file.cjs
./node --experimental-transform-types file.cjs

richardlau · 2024-10-08T17:20:15Z

FWIW, with (and without) this PR I get this:

I think that's because it assumes the string you pass is TS. Can you try the following instead:
echo 'console.log(process.features.typescript)' > file.cjs
./node --experimental-transform-types file.cjs

👍

$ echo 'console.log(process.features.typescript)' > file.cjs
$ ./node --experimental-transform-types file.cjs
false
(node:96078) ExperimentalWarning: Type Stripping is an experimental feature and might change at any time
(Use `node --trace-warnings ...` to show where the warning was created)

I think we probably still want some kind of guard, or maybe make the experimental options conditional on Amaro being used, but that could be done in (yet another) follow on PR.

codecov · 2024-10-08T18:23:46Z

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 88.41%. Comparing base (a05cb0d) to head (60dce1f).
Report is 269 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/internal/bootstrap/node.js	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #55323      +/-   ##
==========================================
- Coverage   88.41%   88.41%   -0.01%     
==========================================
  Files         652      652              
  Lines      186762   186740      -22     
  Branches    36099    36036      -63     
==========================================
- Hits       165123   165099      -24     
- Misses      14902    14908       +6     
+ Partials     6737     6733       -4

Files with missing lines	Coverage Δ
lib/internal/bootstrap/node.js	`99.56% <0.00%> (-0.22%)`	⬇️

... and 58 files with indirect coverage changes

…ilable

marco-ippolito · 2024-10-09T09:58:32Z

I added the guard in this open PR #55282
Like crypto and icu throw an error ERR_NO_TYPESCRIPT when amaro is not available
Maybe I can split it in a different PR

…ilable

nodejs-github-bot · 2024-10-10T07:00:42Z

CI: https://ci.nodejs.org/job/node-test-pull-request/63021/

nodejs-github-bot · 2024-10-10T20:46:07Z

CI: https://ci.nodejs.org/job/node-test-pull-request/63037/

nodejs-github-bot · 2024-10-11T06:53:12Z

Commit Queue failed

- Loading data for nodejs/node/pull/55323
✔  Done loading data for nodejs/node/pull/55323
----------------------------------- PR info ------------------------------------
Title      process: fix `process.features.typescript` when Amaro is unavailable (#55323)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     aduh95:process.features.typescript--without-amaro -> nodejs:main
Labels     process, author ready, needs-ci, strip-types
Commits    3
 - process: fix `process.features.typescript` when Amaro is unavailable
 - fixup! process: fix `process.features.typescript` when Amaro is unava…
 - fixup! process: fix `process.features.typescript` when Amaro is unava…
Committers 1
 - Antoine du Hamel <duhamelantoine1995@gmail.com>
PR-URL: https://github.com/nodejs/node/pull/55323
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/55323
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Tue, 08 Oct 2024 16:49:34 GMT
   ✔  Approvals: 1
   ✔  - Marco Ippolito (@marco-ippolito) (TSC): https://github.com/nodejs/node/pull/55323#pullrequestreview-2359200332
   ✘  This PR needs to wait 105 more hours to land (or 0 hours if there is one more approval)
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2024-10-10T20:46:07Z: https://ci.nodejs.org/job/node-test-pull-request/63037/
- Querying data for job/node-test-pull-request/63037/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/11287733011

test/es-module/test-typescript.mjs

nodejs-github-bot · 2024-10-11T11:27:18Z

Landed in f98d9c1

PR-URL: #55323 Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com>

PR-URL: nodejs#55323 Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com>

process: fix process.features.typescript when Amaro is unavailable

94b422a

nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Oct 8, 2024

RedYetiDev reviewed Oct 8, 2024

View reviewed changes

test/es-module/test-typescript.mjs Outdated Show resolved Hide resolved

RedYetiDev added process Issues and PRs related to the process subsystem. strip-types Issues or PRs related to strip-types support and removed lib / src Issues and PRs related to general changes in the lib or src directory. labels Oct 8, 2024

fixup! process: fix process.features.typescript when Amaro is unava…

dbf4cbe

…ilable

marco-ippolito mentioned this pull request Oct 9, 2024

module: throw ERR_NO_TYPESCRIPT when compiled without amaro #55332

Merged

This comment has been minimized.

Sign in to view

fixup! process: fix process.features.typescript when Amaro is unava…

60dce1f

…ilable

marco-ippolito approved these changes Oct 10, 2024

View reviewed changes

marco-ippolito added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 10, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 10, 2024

aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 10, 2024

marco-ippolito added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 11, 2024

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Oct 11, 2024

marco-ippolito removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Oct 11, 2024

aduh95 requested review from richardlau and legendecas October 11, 2024 08:06

richardlau reviewed Oct 11, 2024

View reviewed changes

test/es-module/test-typescript.mjs Show resolved Hide resolved

richardlau approved these changes Oct 11, 2024

View reviewed changes

marco-ippolito added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 11, 2024

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 11, 2024

nodejs-github-bot merged commit f98d9c1 into nodejs:main Oct 11, 2024
71 checks passed

aduh95 deleted the process.features.typescript--without-amaro branch October 11, 2024 11:28

aduh95 added a commit that referenced this pull request Oct 11, 2024

process: fix process.features.typescript when Amaro is unavailable

0655d3a

PR-URL: #55323 Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com>

aduh95 mentioned this pull request Oct 11, 2024

V22.10.0 proposal #55343

Merged

louwers pushed a commit to louwers/node that referenced this pull request Nov 2, 2024

process: fix process.features.typescript when Amaro is unavailable

4798e05

PR-URL: nodejs#55323 Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com>

marco-ippolito added the dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. label Nov 16, 2024

tpoisseau pushed a commit to tpoisseau/node that referenced this pull request Nov 21, 2024

process: fix process.features.typescript when Amaro is unavailable

4742d53

PR-URL: nodejs#55323 Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

process: fix `process.features.typescript` when Amaro is unavailable #55323

process: fix `process.features.typescript` when Amaro is unavailable #55323

aduh95 commented Oct 8, 2024

nodejs-github-bot commented Oct 8, 2024

richardlau commented Oct 8, 2024

aduh95 commented Oct 8, 2024 •

edited

Loading

richardlau commented Oct 8, 2024 •

edited by aduh95

Loading

codecov bot commented Oct 8, 2024 •

edited

Loading

marco-ippolito commented Oct 9, 2024 •

edited

Loading

This comment has been minimized.

nodejs-github-bot commented Oct 10, 2024

nodejs-github-bot commented Oct 10, 2024

nodejs-github-bot commented Oct 11, 2024

nodejs-github-bot commented Oct 11, 2024

process: fix process.features.typescript when Amaro is unavailable #55323

process: fix process.features.typescript when Amaro is unavailable #55323

Conversation

aduh95 commented Oct 8, 2024

nodejs-github-bot commented Oct 8, 2024

richardlau commented Oct 8, 2024

aduh95 commented Oct 8, 2024 • edited Loading

richardlau commented Oct 8, 2024 • edited by aduh95 Loading

codecov bot commented Oct 8, 2024 • edited Loading

Codecov Report

marco-ippolito commented Oct 9, 2024 • edited Loading

This comment has been minimized.

nodejs-github-bot commented Oct 10, 2024

nodejs-github-bot commented Oct 10, 2024

nodejs-github-bot commented Oct 11, 2024

nodejs-github-bot commented Oct 11, 2024

process: fix `process.features.typescript` when Amaro is unavailable #55323

process: fix `process.features.typescript` when Amaro is unavailable #55323

aduh95 commented Oct 8, 2024 •

edited

Loading

richardlau commented Oct 8, 2024 •

edited by aduh95

Loading

codecov bot commented Oct 8, 2024 •

edited

Loading

marco-ippolito commented Oct 9, 2024 •

edited

Loading